Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken fields #1718

Merged
merged 4 commits into from
Jan 31, 2023
Merged

Fix broken fields #1718

merged 4 commits into from
Jan 31, 2023

Conversation

myieye
Copy link
Collaborator

@myieye myieye commented Jan 27, 2023

Fixes #1717

Description

The LexEntryModel->entryImportResidue and (see: #1720) LexSense->status fields throw exceptions when they're updated, because they're not configured correctly in PHP.

Checklist

  • I have labeled my PR with: bug, feature, engineering, security fix or testing
  • I have performed a self-review of my own code
  • I have reviewed the title & description of this PR which I will use as the squashed PR commit message
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have enabled auto-merge (optional)

QA testing

Testers, use the following instructions on our staging environment. Post your findings as a comment and include any meaningful screenshots, etc.

  • Update the LexSense->status field
  • Wait 5s and confirm no error is displayed

@myieye myieye added the bug An existing problem with our app in production label Jan 27, 2023
@myieye myieye self-assigned this Jan 27, 2023
@myieye myieye enabled auto-merge (squash) January 27, 2023 15:13
@github-actions
Copy link

github-actions bot commented Jan 27, 2023

Unit Test Results

362 tests   362 ✔️  17s ⏱️
  37 suites      0 💤
    1 files        0

Results for commit b9448e3.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@megahirt megahirt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, really impressed that you dug in to actually fix this problem, @myieye ! I am fairly confident that this has been broken for ages, however when I removed the "silently swallow all exceptions" code, it suddenly started showing the error in the client instead of failing silently.

Looks like the unit tests need tweaking with this change. Otherwise LGTM.

@myieye
Copy link
Collaborator Author

myieye commented Jan 31, 2023

So, I went to expand the tests to cover the missing entryImportResidue field and discovered that it was intentionally removed🤔.

image

So...I guess it's a mistake that we show it in the UI.

senseImportResidue on the other hand was never removed from our models, but is ignored when decoding lift files (as is the entryImportResidue). So, I'm guessing we want to just remove both from our models and the UI.

image

@myieye myieye merged commit 4ef01fe into develop Jan 31, 2023
@myieye myieye deleted the bug/broken-entrysense-properties branch January 31, 2023 15:14
@longrunningprocess
Copy link
Contributor

Testing

entry's import residue ❌ (FYI: @myieye )

image

meaning's status ✅

image

@myieye
Copy link
Collaborator Author

myieye commented Feb 2, 2023

@longrunningprocess thanks for testing!

I had updated the issue, but forgotten to update the PR description: This PR no longer fixes the import-residue field, because it probably shouldn't exist.

Your testing is green for what I did fix, so I sneakily moved this to POSO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An existing problem with our app in production
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken entry/sense properties
3 participants